Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pos-only params in tkinter module #11506

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Fix pos-only params in tkinter module #11506

merged 2 commits into from
Feb 29, 2024

Conversation

sobolevn
Copy link
Member

Same as #11505

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood

This comment was marked as resolved.

@AlexWaygood AlexWaygood merged commit 7b4c751 into main Feb 29, 2024
57 checks passed
@AlexWaygood AlexWaygood deleted the pos-only-tkinter branch February 29, 2024 09:02
@Akuli
Copy link
Collaborator

Akuli commented Feb 29, 2024

This is less readable. What problem does this solve?

@AlexWaygood
Copy link
Member

AlexWaygood commented Feb 29, 2024

This is less readable. What problem does this solve?

See conversation in #11505 (comment). I had the same initial reaction, but pyright's error is, strictly speaking, correct here. It's invalid to have a pos-only parameter following a pos-or-keyword parameter, and stubtest correctly complains about the alternative solution I thought of.

@Akuli
Copy link
Collaborator

Akuli commented Feb 29, 2024

I still don't see a practical problem being solved. Type checkers will generally tolerate small issues in stubs as long as they can understand what the stubs are trying to say. On the other hand, IDE users will not be happy trying to decipher the new Unpack contraption.

@AlexWaygood
Copy link
Member

I'd be okay reverting if we add test cases to make sure type checkers understand that it's valid at runtime to pass default using a keyword argument. But note that there's no way of directly translating what we had before this PR into PEP-570 syntax -- with PEP-570, all parameters before the / are positional-only. And we're hoping to soon move to PEP-570 everywhere (#11250).

@Akuli
Copy link
Collaborator

Akuli commented Feb 29, 2024

it's valid at runtime to pass default using a keyword argument

It's not. This method cannot be called so that you pass a keyword argument for default. If you try to do it, python will complain that you're passing two values for it:

>>> import tkinter
>>> root = tkinter.Tk()
>>> root.wm_iconphoto('lol', default=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Wm.wm_iconphoto() got multiple values for argument 'default'

I now see that the old way to write the stub with default and __image1 is wrong/misleading, but I still don't like what this PR did.

@AlexWaygood
Copy link
Member

AlexWaygood commented Feb 29, 2024

it's valid at runtime to pass default using a keyword argument

It's not. This method cannot be called so that you pass a keyword argument for default. If you try to do it, python will complain that you're passing two values for it:

Got it -- my mistake. FWIW, I did test locally, but obviously not thoroughly enough :)

In that case, we should be able to simply update the signature to the much-simpler

def wm_iconphoto(self, default: bool, image1: _PhotoImageLike | str, /, *args: _PhotoImageLike | str) -> None: ...

@Akuli
Copy link
Collaborator

Akuli commented Feb 29, 2024

FWIW, I did test locally, but obviously not thoroughly enough :)

To be honest, the whole wm_iconphoto() method is a mess. A positional-only boolean parameter is just a bad idea for readability, but it's too late to change it now: that would break every tkinter program that sets a custom icon for the app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants